-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Locals in exclusive systems #3946
Locals in exclusive systems #3946
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this functionality a great deal: it opens up some very useful patterns (particularly around storing system state and schedules). Code quality looks good, but this needs a test or two to verify that it works.
I would also like a doc test to demonstrate this, probably on the ExclusiveSystem
trait where it's most discoverable.
P.S. This should play nicely with the plans to make exclusive systems less special in Stageless; although I suspect we'll be able to cut a lot of the code needed here.
/// Just a simple exclusive system - this function will run with mutable access to | ||
/// the main app world. This lets it call into other schedules, or modify and query the | ||
/// world in hard-to-predict ways, which makes it a powerful primitive. However, because | ||
/// this is usually not needed, and because such wide access makes parallelism impossible, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain why not -> should be avoided when not needed.
/// as mostly-normal systems but with the ability to change parameter sets and flush commands midway through. | ||
fn stateful_exclusive_system( | ||
world: &mut World, | ||
mut part_one_state: Local<SystemState<(SRes<u32>, SCommands)>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mut part_one_state: Local<SystemState<(SRes<u32>, SCommands)>>, | |
mut part_one_state: Local<SystemState<(Res<u32>, Commands)>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the SystemState
API needs the lifetimeless versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no. Please leave a comment explaining this then.
That surprises me a lot though; I request standard Res
through the SystemState
API in exclusive systems all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the problem is the systemstate api; it's that the Local
s value must (correctly) be 'static
, but a type with non-'static
type paramaters cannot itself be `'static' (even though the lifetime cannot be used. I run into a similar issue in #3817.
That is, the issue is Local
not SystemState
; and the issue in Local
is 'due to' our SystemParam
design. I don't think this is avoidable.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
fn main() { | ||
App::new() | ||
.init_resource::<MyCustomSchedule>() | ||
.insert_resource(5u32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use wrapper types, even in examples.
mut part_one_state: Local<SystemState<(SRes<u32>, SCommands)>>, | ||
mut part_two_state: Local<SystemState<SQuery<Read<MyComponent>>>>, | ||
) { | ||
let (resource, mut commands) = part_one_state.get(world); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh fascinating: this "just works" without initial values because of the FromWorld
impl 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a bit to follow the macro, but otherwise a rather succinct and useful change here.
Running into this when implementing animation exclusive systems that needs query caching.
} | ||
|
||
all_tuples!(impl_exclusive_system, 0, 12, L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all_tuples!(impl_exclusive_system, 0, 12, L); | |
all_tuples!(impl_exclusive_system, 0, 16, L); |
I would rather have all all_tuples work on the same maximum number of items in a tuple, so 16. It would be less surprising for users
}; | ||
} | ||
|
||
all_tuples!(impl_from_world, 0, 12, T); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Closing in favor of #4166 |
…arams (#6083) # Objective The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move. This is an alternative to #4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns). ## Solution This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). This means you can remove all cases of `exclusive_system()`: ```rust // before commands.add_system(some_system.exclusive_system()); // after commands.add_system(some_system); ``` I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems: ```rust fn some_exclusive_system( world: &mut World, transforms: &mut QueryState<&Transform>, state: &mut SystemState<(Res<Time>, Query<&Player>)>, ) { for transform in transforms.iter(world) { println!("{transform:?}"); } let (time, players) = state.get(world); for player in players.iter() { println!("{player:?}"); } } ``` Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system. I added some targeted SystemParam `static` constraints, which removed the need for this: ``` rust fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {} ``` ## Related - #2923 - #3001 - #3946 ## Changelog - `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait. - `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems - `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam` - Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api. ## Migration Guide Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems: ```rust // Old (0.8) app.add_system(some_exclusive_system.exclusive_system()); // New (0.9) app.add_system(some_exclusive_system); ``` Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis: ```rust // Old (0.8) app.add_system(some_system.exclusive_system().at_end()); // New (0.9) app.add_system(some_system.at_end()); ``` Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons: ```rust // Old (0.8) fn some_system(world: &mut World) { let mut transforms = world.query::<&Transform>(); for transform in transforms.iter(world) { } } // New (0.9) fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) { for transform in transforms.iter(world) { } } ```
…arams (bevyengine#6083) # Objective The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move. This is an alternative to bevyengine#4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns). ## Solution This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). This means you can remove all cases of `exclusive_system()`: ```rust // before commands.add_system(some_system.exclusive_system()); // after commands.add_system(some_system); ``` I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems: ```rust fn some_exclusive_system( world: &mut World, transforms: &mut QueryState<&Transform>, state: &mut SystemState<(Res<Time>, Query<&Player>)>, ) { for transform in transforms.iter(world) { println!("{transform:?}"); } let (time, players) = state.get(world); for player in players.iter() { println!("{player:?}"); } } ``` Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system. I added some targeted SystemParam `static` constraints, which removed the need for this: ``` rust fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {} ``` ## Related - bevyengine#2923 - bevyengine#3001 - bevyengine#3946 ## Changelog - `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait. - `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems - `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam` - Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api. ## Migration Guide Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems: ```rust // Old (0.8) app.add_system(some_exclusive_system.exclusive_system()); // New (0.9) app.add_system(some_exclusive_system); ``` Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis: ```rust // Old (0.8) app.add_system(some_system.exclusive_system().at_end()); // New (0.9) app.add_system(some_system.at_end()); ``` Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons: ```rust // Old (0.8) fn some_system(world: &mut World) { let mut transforms = world.query::<&Transform>(); for transform in transforms.iter(world) { } } // New (0.9) fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) { for transform in transforms.iter(world) { } } ```
…arams (bevyengine#6083) # Objective The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move. This is an alternative to bevyengine#4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns). ## Solution This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). This means you can remove all cases of `exclusive_system()`: ```rust // before commands.add_system(some_system.exclusive_system()); // after commands.add_system(some_system); ``` I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems: ```rust fn some_exclusive_system( world: &mut World, transforms: &mut QueryState<&Transform>, state: &mut SystemState<(Res<Time>, Query<&Player>)>, ) { for transform in transforms.iter(world) { println!("{transform:?}"); } let (time, players) = state.get(world); for player in players.iter() { println!("{player:?}"); } } ``` Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system. I added some targeted SystemParam `static` constraints, which removed the need for this: ``` rust fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {} ``` ## Related - bevyengine#2923 - bevyengine#3001 - bevyengine#3946 ## Changelog - `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait. - `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems - `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam` - Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api. ## Migration Guide Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems: ```rust // Old (0.8) app.add_system(some_exclusive_system.exclusive_system()); // New (0.9) app.add_system(some_exclusive_system); ``` Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis: ```rust // Old (0.8) app.add_system(some_system.exclusive_system().at_end()); // New (0.9) app.add_system(some_system.at_end()); ``` Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons: ```rust // Old (0.8) fn some_system(world: &mut World) { let mut transforms = world.query::<&Transform>(); for transform in transforms.iter(world) { } } // New (0.9) fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) { for transform in transforms.iter(world) { } } ```
…arams (bevyengine#6083) # Objective The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move. This is an alternative to bevyengine#4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns). ## Solution This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). This means you can remove all cases of `exclusive_system()`: ```rust // before commands.add_system(some_system.exclusive_system()); // after commands.add_system(some_system); ``` I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems: ```rust fn some_exclusive_system( world: &mut World, transforms: &mut QueryState<&Transform>, state: &mut SystemState<(Res<Time>, Query<&Player>)>, ) { for transform in transforms.iter(world) { println!("{transform:?}"); } let (time, players) = state.get(world); for player in players.iter() { println!("{player:?}"); } } ``` Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system. I added some targeted SystemParam `static` constraints, which removed the need for this: ``` rust fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {} ``` ## Related - bevyengine#2923 - bevyengine#3001 - bevyengine#3946 ## Changelog - `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait. - `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems - `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam` - Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api. ## Migration Guide Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems: ```rust // Old (0.8) app.add_system(some_exclusive_system.exclusive_system()); // New (0.9) app.add_system(some_exclusive_system); ``` Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis: ```rust // Old (0.8) app.add_system(some_system.exclusive_system().at_end()); // New (0.9) app.add_system(some_system.at_end()); ``` Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons: ```rust // Old (0.8) fn some_system(world: &mut World) { let mut transforms = world.query::<&Transform>(); for transform in transforms.iter(world) { } } // New (0.9) fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) { for transform in transforms.iter(world) { } } ```
Objective
Make it easier to have stateful exclusive systems
Solution
Add
Local
support to exclusive systems.Note: this PR is a follow up to #3945, and while it's not strictly required, it's highly recommended, and this PR is branched off of it.